rewrite.rs: refactor new_parents to depend only on parent_mapping
#2646
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 tasks
ilyagr
added
help wanted
ilyagr
force-pushed
the
refactor1
branch
3 times, most recently
from
6a6f499 to
ea7f3f5
Compare
martinvonz
reviewed
Nov 28, 2023
ilyagr
force-pushed
the
refactor1
branch
3 times, most recently
from
59ef635 to
149df31
Compare
yuja
reviewed
Nov 28, 2023
ilyagr
commented
Nov 28, 2023
ilyagr
force-pushed
the
refactor1
branch
9 times, most recently
from
c5f1b89 to
fa5fd1a
Compare
|
I added a few comments. I realized that, for other uses, the assertion that replacements are non-empty might be unnecessary, but I'm not sure when if ever other uses might come up. |
ilyagr
added a commit
to ilyagr/jj
that referenced
this pull request
Dec 1, 2023
See comments inline for details. In particular, I wanted to make sure these behaviors are not affected by jj-vcs#2646. They don't seem to be.
4 tasks
ilyagr
added a commit
to ilyagr/jj
that referenced
this pull request
Dec 1, 2023
See comments inline for details. Cc jj-vcs#2600. In particular, I wanted to make sure these behaviors are not affected by jj-vcs#2646. They don't seem to be.
ilyagr
added a commit
to ilyagr/jj
that referenced
this pull request
Dec 1, 2023
See comments inline for details. Cc jj-vcs#2600. In particular, I wanted to make sure these behaviors are not affected by jj-vcs#2646. They don't seem to be. The tests ended up weirder than expected because of jj-vcs#2600 (comment). Even though, for now, the behavior of tests is unaffected by that issue, the *expected* behavior is different.
ilyagr
added a commit
to ilyagr/jj
that referenced
this pull request
Dec 1, 2023
See comments inline for details. Cc jj-vcs#2600. In particular, I wanted to make sure these behaviors are not affected by jj-vcs#2646. They don't seem to be. The tests ended up weirder than expected because of jj-vcs#2600 (comment). Even though, right now, the behavior of tests is unaffected by that issue, the *expected* behavior is different.
ilyagr
changed the title
new_parents to depend only on parent_mappingrewrite.rs: refactor new_parents to depend only on parent_mapping
yuja
approved these changes
Dec 1, 2023
ilyagr
added a commit
that referenced
this pull request
Dec 2, 2023
See comments inline for details. Cc #2600. In particular, I wanted to make sure these behaviors are not affected by #2646. They don't seem to be. The tests ended up weirder than expected because of #2600 (comment). Even though, right now, the behavior of tests is unaffected by that issue, the *expected* behavior is different.
|
I'm thinking of maybe waiting until the next release (which is, I'm guessing, planned for Wednesday) before merging this, since I doubt I'll have time to implement any user-visible bugfixes or features that use this PR before that. |
ilyagr
force-pushed
the
refactor1
branch
3 times, most recently
from
f237963 to
8dd1e63
Compare
ilyagr
added a commit
to ilyagr/jj
that referenced
this pull request
Dec 12, 2023
As suggested by @yuja in jj-vcs#2646 (comment)
ilyagr
added a commit
to ilyagr/jj
that referenced
this pull request
Dec 12, 2023
As suggested by @yuja in jj-vcs#2646 (comment)
|
Planning to merge this in ~24 hours. |
ilyagr
added a commit
to ilyagr/jj
that referenced
this pull request
Dec 12, 2023
As suggested by @yuja in jj-vcs#2646 (comment)
Previously, the function relied on both the `self.parent_mapping` and `self.rebased`. If `(A,B)` was in `parent_mapping` and `(B,C)` was in `rebased`, `new_parents` would map `A` to `C`. Now, `self.rebased` is ignored by `new_parents`. In the same situation, DescendantRebaser is changed so that both `(A,B)` and `(B,C)` are in `parent_mapping` before. `new_parents` now applies `parent_mapping` repeatedly, and will map `A` to `C` in this situation. ## Cons - The semantics are changed; `new_parents` now panics if `self.parent_mapping` contain cycles. AFAICT, such cycles never happen in `jj` anyway, except for one test that I had to fix. I think it's a sensible restriction to live with; if you do want to swap children of two commits, you can call `rebase_descendants` twice. ## Pros - I find the new logic much easier to reason about. I plan to extract it into a function, to be used in refactors for `jj rebase -r` and `jj new --after`. It will make it much easier to have a correct implementation of `jj rebase -r --after`, even when rebasing onto a descendant. - The de-duplication is no longer O(n^2). I tried to keep the common case fast. ## Alternatives - We could make `jj rebase` and `jj new` use a separate function with the algorithm shown here, without changing DescendantRebaser. I believe that the new algorithm makes DescendatRebaser easier to understand, though, and it feels more elegant to reduce code duplication. - The de-duplication optimization here is independent of other changes, and could be used on its own.
ilyagr
added a commit
to ilyagr/jj
that referenced
this pull request
Dec 13, 2023
As suggested by @yuja in jj-vcs#2646 (comment)
ilyagr
added a commit
to ilyagr/jj
that referenced
this pull request
Dec 13, 2023
As suggested by @yuja in jj-vcs#2646 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Notes
This could be reviewed now or together with the other refactors that would happen in subsequent commits (making
new_parentsa public function, etc). If you prefer the latter, let me know. You can also look at https://github.com/ilyagr/jj/tree/refactor, but it's stillunpolished.
I'm posting this mainly to illustrate my plans, and so that you can let me know if there is some fatal problem with them.
Commit description
Previously, the function relied on both the
self.parent_mappingandself.rebased. If(A,B)was inparent_mappingand(B,C)was inrebased,new_parentswould mapAtoC.Now,
self.rebasedis ignored bynew_parents. In the same situation, DescendantRebaser is changed so that both(A,B)and(B,C)are inparent_mappingbefore.new_parentsnow appliesparent_mappingrepeatedly, and will mapAtoCin this situation.Cons
new_parentsnow panics ifself.parent_mappingcontain cycles. AFAICT, such cycles never happen injjanyway, except for one test that I had to fix. I think it's a sensible restriction to live with; if you do want to swap children of two commits, you can callrebase_descendantstwice.Pros
I find the new logic much easier to reason about. I plan to extract it into a function, to be used in refactors for
jj rebase -randjj new --after. It will make it much easier to have a correct implementation ofjj rebase -r --after, even when rebasing onto a descendant.The de-duplication is no longer O(n^2). I tried to keep the common case fast.
Alternatives
We could make
jj rebaseandjj newuse a separate function with the algorithm shown here, without changing DescendantRebaser. I believe that the new algorithm makes DescendatRebaser easier to understand, though, and it feels more elegant to reduce code duplication.The de-duplication optimization here is independent of other changes, and could be used on its own.
Checklist
If applicable:
CHANGELOG.md